Skip to content

Allow to tweak default scopes for accounts#31623

Merged
juliusknorr merged 1 commit intomasterfrom
tweak-default-scopes-accounts
May 17, 2022
Merged

Allow to tweak default scopes for accounts#31623
juliusknorr merged 1 commit intomasterfrom
tweak-default-scopes-accounts

Conversation

@tcitworld
Copy link
Member

@tcitworld tcitworld commented Mar 18, 2022

Closes #6582, Closes #14358, Closes #14959

Replaces #20667

@tcitworld tcitworld added enhancement 3. to review Waiting for reviews feature: users and groups php Pull requests that update Php code labels Mar 18, 2022
@tcitworld tcitworld added this to the Nextcloud 24 milestone Mar 18, 2022
@tcitworld tcitworld mentioned this pull request Mar 18, 2022
Closed
@tcitworld tcitworld force-pushed the tweak-default-scopes-accounts branch from 55dcbcf to d9c879a Compare March 18, 2022 19:26
@PVince81 PVince81 requested a review from jancborchardt March 22, 2022 09:53
@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
Merged
@blizzz blizzz mentioned this pull request Mar 31, 2022
Merged
This was referenced Apr 7, 2022
Merged
Merged
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
@szaimen szaimen mentioned this pull request Apr 28, 2022
Open
@szaimen
Copy link
Contributor

szaimen commented Apr 28, 2022 •
edited
Loading

I like this PR 👍
But I fear I am not the person to decide... we can push forward with this :)

@szaimen
Copy link
Contributor

szaimen commented Apr 28, 2022

/backport to stable24

@szaimen
Copy link
Contributor

szaimen commented Apr 28, 2022

/backport to stable23

@szaimen
Copy link
Contributor

szaimen commented Apr 28, 2022

@tcitworld can you please resolve the conflicts? Thank you :)

@tcitworld tcitworld force-pushed the tweak-default-scopes-accounts branch from d9c879a to 256237c Compare April 29, 2022 15:03
@tcitworld
Copy link
Member Author

tcitworld commented Apr 29, 2022

Done

@szaimen
Copy link
Contributor

szaimen commented Apr 29, 2022

Thanks! :)

juliusknorr
juliusknorr reviewed Apr 29, 2022
View reviewed changes
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments, otherwise this looks good to me 👍

Pytal
Pytal reviewed Apr 29, 2022
View reviewed changes
Copy link
Member

@Pytal Pytal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔭

@tcitworld tcitworld force-pushed the tweak-default-scopes-accounts branch from 256237c to fa68b55 Compare May 12, 2022 16:26
@tcitworld tcitworld requested review from Pytal and juliusknorr May 12, 2022 16:26
@Pytal
Copy link
Member

Pytal commented May 16, 2022

Could we prevent erroneously allowing displayname and email to be set to private in the logic? Unsupported as per

server/lib/private/Accounts/AccountManager.php

Lines 218 to 228 in 778db45

if (
$property->getScope() === self::SCOPE_PRIVATE
&& in_array($property->getName(), [self::PROPERTY_DISPLAYNAME, self::PROPERTY_EMAIL])
) {
if ($throwOnData) {
// v2-private is not available for these fields
throw new InvalidArgumentException('scope');
} else {
// default to local
$property->setScope(self::SCOPE_LOCAL);
}

In these cases we should default to local scope

@jancborchardt
Copy link
Member

jancborchardt commented May 16, 2022

@tcitworld can you add before/after screenshots for easier design review? Thanks! :)

@tcitworld
Allow to tweak default scopes for accounts
4d26a9a 
Close #6582

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
@tcitworld tcitworld force-pushed the tweak-default-scopes-accounts branch from fa68b55 to 4d26a9a Compare May 16, 2022 20:55
@tcitworld
Copy link
Member Author

tcitworld commented May 16, 2022

Could we prevent erroneously allowing displayname and email to be set to private in the logic?

Done!

can you add before/after screenshots for easier design review?

I really can't ! 😅
This is a new advanced setting for administrators through config.php only.

@juliusknorr
Copy link
Member

juliusknorr commented May 17, 2022

Failures are unrelated.

@juliusknorr juliusknorr merged commit 4c61db4 into master May 17, 2022
@juliusknorr juliusknorr deleted the tweak-default-scopes-accounts branch May 17, 2022 06:23
This was referenced May 17, 2022
Closed
Closed
@szaimen szaimen added the pending documentation This pull request needs an associated documentation update label May 17, 2022
@szaimen
Copy link
Contributor

szaimen commented May 17, 2022

Should we add some documentation on this with a complete example?

@tcitworld
Copy link
Member Author

tcitworld commented May 17, 2022 •
edited
Loading

Yup, needs an entry in https://docs.nextcloud.com/server/latest/admin_manual/configuration_server/config_sample_php_parameters.html

I would suggest a new section with profile.enabled.

@szaimen
Copy link
Contributor

szaimen commented May 17, 2022

There is also this: https://docs.nextcloud.com/server/latest/admin_manual/configuration_user/profile_configuration.html
Do you think we could simply add it to this section? :)

@tcitworld
Copy link
Member Author

tcitworld commented May 17, 2022

Yes, more appropriate. But still needs a quick mention just before https://docs.nextcloud.com/server/latest/admin_manual/configuration_server/config_sample_php_parameters.html#app-config-options

@szaimen
Copy link
Contributor

szaimen commented May 17, 2022

But still needs a quick mention just before https://docs.nextcloud.com/server/latest/admin_manual/configuration_server/config_sample_php_parameters.html#app-config-options

If I understood this section in the settings documentation correctly, it is actually auto-generated. So I think we should not need to edit this section manually? Maybe @artonge knows?

@szaimen
Copy link
Contributor

szaimen commented May 17, 2022

But of course the profile_configuration section should then be edited and include this setting, I suppose :)

@tcitworld
Copy link
Member Author

tcitworld commented May 17, 2022

Oh right, I missed that !

@szaimen
Copy link
Contributor

szaimen commented May 17, 2022

@tcitworld can you open a PR with some documentation on this over at https://github.com/nextcloud/documentation/edit/master/admin_manual/configuration_user/profile_configuration.rst or shall I try to add some documentation on this? :)

@tcitworld tcitworld mentioned this pull request May 17, 2022
Merged
@tcitworld
Copy link
Member Author

tcitworld commented May 17, 2022

Done in nextcloud/documentation#8320

@szaimen
Copy link
Contributor

szaimen commented May 17, 2022

Thank you! 🚀

@szaimen szaimen removed the pending documentation This pull request needs an associated documentation update label May 17, 2022
@SergioArbarviro SergioArbarviro mentioned this pull request Oct 11, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@juliusknorr juliusknorr juliusknorr approved these changes

@Pytal Pytal Pytal approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@blizzz blizzz Awaiting requested review from blizzz

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

Assignees

No one assigned

Labels

3. to review Waiting for reviews enhancement feature: users and groups php Pull requests that update Php code

Projects

None yet

Milestone

Nextcloud 25

Development

Successfully merging this pull request may close these issues.

don't reveal users mail adress by default set profile to private/public on new user creation Tweak the default scopes for Accounts

6 participants

@tcitworld @szaimen @Pytal @jancborchardt @juliusknorr @blizzz